Conversation
WalkthroughA concurrency control mechanism was introduced within the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant ExceptionHandler
Client->>Database: createOrUpdateDocumentsWithIncrease(doc, timestamp)
Database->>Database: Validate document structure
Database->>Database: Parse oldDoc.$updatedAt
alt Parsing fails
Database->>ExceptionHandler: Throw DatabaseException
else Parsing succeeds
alt oldDoc.$updatedAt > request timestamp
Database->>ExceptionHandler: Throw ConflictException
else No conflict
Database->>Database: Proceed with create/update logic
end
end
Database-->>Client: Result or Exception
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
src/Database/Database.php (2)
4257-4260: WrapDateTimeconstruction in try/catch for consistency & safetyOther new conflict-detection blocks (
updateDocuments,createOrUpdateDocumentsWithIncrease,delete*) correctly guard thenew \DateTime()call with atry … catch.
Here the same call is left unguarded – an invalid or empty$updatedAtstring will bubble up as a fatalException.- $oldUpdatedAt = new \DateTime($old->getUpdatedAt()); + try { + $oldUpdatedAt = new \DateTime($old->getUpdatedAt()); + } catch (\Throwable $e) { + throw new DatabaseException($e->getMessage(), $e->getCode(), $e); + }
4430-4438: Conflict check is performed on the modified document – never firesIn this loop the variable
$documentis mutated before the timestamp comparison:$new = new Document(array_merge($document->getArrayCopy(), $updates->getArrayCopy())); $document = $new; // <— updatedAt already overwritten (now) ... $oldUpdatedAt = new \DateTime($document->getUpdatedAt()); // uses NEW timestampAs a result the
ConflictExceptioncannot trigger because the freshly-set$updatedAtis always ≥$this->timestamp.Quick fix – capture and test the original instance:
foreach ($batch as &$document) { - $new = new Document(array_merge($document->getArrayCopy(), $updates->getArrayCopy())); - $document = $new; - try { - $oldUpdatedAt = new \DateTime($document->getUpdatedAt()); + $originalUpdatedAt = $document->getUpdatedAt(); + + try { + $oldUpdatedAt = new \DateTime($originalUpdatedAt); } catch (Exception $e) { ... } if (!is_null($this->timestamp) && $oldUpdatedAt > $this->timestamp) { throw new ConflictException('Document was updated after the request timestamp'); } + + $new = new Document(array_merge($document->getArrayCopy(), $updates->getArrayCopy())); + $document = $new;(or move the existing block above the re-assignment).
Without this fix stale-write protection is ineffective in
updateDocuments.
🧹 Nitpick comments (1)
src/Database/Database.php (1)
5862-5869: Conflict gate for bulk deletes works but could be hardenedImplementation matches other sections. Consider extracting the repeated “parse-and-compare updatedAt” code into a small private helper to reduce duplication and ensure future fixes (like time-zone handling) are applied everywhere at once.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Database.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Setup & Build Docker Image
- GitHub Check: CodeQL
🔇 Additional comments (2)
src/Database/Database.php (2)
5030-5038: Looks good – identical guard added to upsert pathThe timestamp-based conflict detection is implemented correctly here (uses
$old, is wrapped intry … catch, and matches the logic used elsewhere).
5341-5349: Guard added todeleteDocumentis saneThe new check mirrors the pattern used in creation/update paths and prevents accidental deletion of fresher data. No issues spotted.
1 participant
Summary by CodeRabbit